remove cast by type after apply JEXL expression #1034
Conversation
AlvaroVega
changed the title
AlvaroVega
changed the title
| } | ||
|
|
||
| newAttribute.value = applyExpression(attribute.expression, context, typeInformation); | ||
|
|
There was a problem hiding this comment.
Is there any piece of documentation about this autocast feature that should be also removed/modified?
There was a problem hiding this comment.
There is nothing about this autocast in https://github.com/telefonicaid/iotagent-node-lib/blob/master/doc/expressionLanguage.md#jexl-based-transformations
| @@ -192,7 +192,7 @@ describe('Combine Jexl and legacy expressions (default JEXL)', function () { | |||
| .matchHeader('fiware-servicepath', 'gardens') | |||
There was a problem hiding this comment.
We have also a variant of this tests for ngsiv2
test/unit/ngsiv2/expressions/expressionCombinedTransformations-test.js
That file doesn't need modifications? (looking to CI results, it seems it doesn't, but I wonder why...)
There was a problem hiding this comment.
expressionCombinedTransformations-test.js includes tests which legacy and jexl expression, but not any of now not allowed cases:
Update for an integer attribute with string expression
Update for a Float attribute with string expression
Update for a Null attribute with arithmetic expression
Update for a Null attribute with string expression
Update for a Boolean attribute with arithmetic expression
Update for a Boolean attribute with string expression
There was a problem hiding this comment.
I have had a look into this with more detail.
In test/unit/expressions/expressionCombinedTransformations-test.js (NGSIv1 variant) the only difference between old files (e.g. test/unit/examples/contextRequests/updateContextExpressionPlugin3.json) and new files (test/unit/examples/contextRequests/updateContextExpressionPlugin8.json) is that the old uses "value": "1040" ("stringfied") and the new "value": 1040 ("native").
On the other hand test/unit/ngsiv2/expressions/expressionCombinedTransformations-test.js (NGSIv2 variant) already uses the native way, e.g. see test/unit/ngsiv2/examples/contextRequests/updateContextExpressionPlugin1.json.
Thus, I think that explains that only test/unit/expressions/expressionCombinedTransformations-test.js is changed in this PR but not test/unit/ngsiv2/expressions/expressionCombinedTransformations-test.js
NTC
| }); | ||
| }); | ||
| }); | ||
| // Deleted test by Issue #1034 |
There was a problem hiding this comment.
If the "autocast after jexl processing" removal is definitive, maybe it's better to remove these tests instead of commenting. What do you think?
CHANGES_NEXT_RELEASE
Outdated
| @@ -1,3 +1,4 @@ | |||
| - Fix: Do not transform attribute value using attribute type after apply expression plugin JEXL (#1034) | |||
There was a problem hiding this comment.
#1034 is the number of the PR itself. I understand that in this case we don't have any issue, so the PR is cited, right?
There was a problem hiding this comment.
fixed with issue number 1036
| .post( | ||
| '/v1/updateContext', | ||
| utils.readExampleFile('./test/unit/examples/contextRequests/updateContextExpressionPlugin3.json') | ||
| utils.readExampleFile('./test/unit/examples/contextRequests/updateContextExpressionPlugin3b.json') |
There was a problem hiding this comment.
Instead of creating a new file (with the b suffix) I think it would be getter to edit the current ones. This way, the removal done in PR #995 would be easy (as there files are currently taken into account for the removal).
There was a problem hiding this comment.
IMHO is not possible edit current file, and should be splited to use in different tests.
Renamed files done into 8f3ea37
There was a problem hiding this comment.
Ok. I see (for instance https://github.com/telefonicaid/iotagent-node-lib/search?q=updateContextExpressionPlugin3).
I'll comment about the new files added in this PR on #995
There was a problem hiding this comment.
issue #1036
The following tests are now not possible: